Skip to content

CYF - London | Anna Fedyna | Module-Structuring-and-Testing-Data | Week 3 #91

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

annafedyna
Copy link

@annafedyna annafedyna commented Nov 6, 2024

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

I’d appreciate it if you could review whether my input checks are efficient, especially when checking if a value is a number in get-angle-type.test.js !

@annafedyna annafedyna changed the title CYF - Lndon | Anna Fedyna | Module-Structuring-and-Testing-Data | Week 2 CYF - London | Anna Fedyna | Module-Structuring-and-Testing-Data | Week 2 Nov 6, 2024
@annafedyna annafedyna changed the title CYF - London | Anna Fedyna | Module-Structuring-and-Testing-Data | Week 2 CYF - London | Anna Fedyna | Module-Structuring-and-Testing-Data | Week 3 Nov 6, 2024
@annafedyna annafedyna added the Needs Review Participant to add when requesting review label Nov 6, 2024
Copy link

@CE-0 CE-0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left you some feedback.

const hourClockTime12 = "20:53";
const hourClockTime24 = "08:53";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment of problem is correct, but there seem further issues with the original variable declarations. I suggest taking a closer at the values of the variables in relation to the variable names to find the issue.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, thank you, the variable names and values were mismatched.
It should be :
const hourClockTime24 = "20:53"; const hourClockTime12 = "08:53";

@@ -4,3 +4,5 @@ count = count + 1;

// Line 1 is a variable declaration, creating the count variable with an initial value of 0
// Describe what line 3 is doing, in particular focus on what = is doing

/* the = operator here is taking the result of the expression on the right and assigning it to variable count */
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it would help to provide slightly more explanation as to what's happening here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, I will add here a bit more detailed explanation about what's happening with the = operator in JavaScript. So, in line 3, the = operator is used to assign the value on the right-hand side to the variable on the left-hand side. We have count = count + 1 . First, the value of count (which is initially 0) is accessed. Then, 1 is added to it, resulting in 1.This value (1) is assigned back to the variable count. The = operator here doesn't mean "equal" . Instead, it acts as an assignment operator which updates the variable with the new value.

@@ -7,3 +7,11 @@ const num = 56.5678;
// Create a variable called roundedNum and assign to it an expression that evaluates to 57 ( num rounded to the nearest whole number )

// Log your variables to the console to check your answers

const wholeNumberPart = Math.floor(num);
const decimalPart = num - Math.floor(num);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not give the expected result, as it has too many decimal places. Perhaps there is a way to limit them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use toFixed() to limit decimal places




console.log(roundedNum);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were asked to log all the variables, not just one.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing out!

// Create a variable to store the ext part of the variable
const extPart = base.slice(base.lastIndexOf('.')+1)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at the diagram above and see if it matches up exactly with your solution.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, after reviewing the diagram and running the code it matches, please let me know if solution not aligns correctly

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take note of all the characters under the 'base' column

/*
(maximum - minimum + 1) = 100
Math.random() - generates a random floating-point number between (0, 1)
Math.floor() - rounds down to the nearest whole number
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the necessity for rounding down rather than rounding up?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that num is not bigger than 100

@@ -7,3 +7,15 @@ const num = Math.floor(Math.random() * (maximum - minimum + 1)) + minimum;
// Try breaking down the expression and using documentation to explain what it means
// It will help to think about the order in which expressions are evaluated
// Try logging the value of num and running the program several times to build an idea of what the program is doing

/*
(maximum - minimum + 1) = 100
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose behind the multiplication?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose is to scale the random value

@cjyuan
Copy link
Contributor

cjyuan commented Nov 10, 2024

I suppose this PR is for sprint-3 (since it says week 3)?
However, the current branch was created from Sprint-2 branch, and not from main. As a result, it contain modified files in both Sprint-1 and Sprint=2. Can you try to rebase the current branch so that it is branched out from main?

Apparently, this is a common mistake. See if can fix the issue from the suggestions found on Slack:
https://codeyourfutur-yov6609.slack.com/archives/C07PVEVQG9X/p1731018581934559

@cjyuan cjyuan added 👀 Review Git Changes requested to do with Git and removed Needs Review Participant to add when requesting review labels Nov 10, 2024
@annafedyna
Copy link
Author

I suppose this PR is for sprint-3 (since it says week 3)? However, the current branch was created from Sprint-2 branch, and not from main. As a result, it contain modified files in both Sprint-1 and Sprint=2. Can you try to rebase the current branch so that it is branched out from main?

Apparently, this is a common mistake. See if can fix the issue from the suggestions found on Slack: https://codeyourfutur-yov6609.slack.com/archives/C07PVEVQG9X/p1731018581934559

Thank you very much for pointing out ! I rebased two branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 Review Git Changes requested to do with Git
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants